feat(vmcp): log active sessions on creation, expiry, and periodically#4019
feat(vmcp): log active sessions on creation, expiry, and periodically#4019
Conversation
yrobla
requested review from
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00fd4857c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds session visibility to the vMCP /status endpoint to support operational debugging and compliance auditing when SessionManagementV2 is enabled.
Changes:
- Extend
/statusresponse schema to optionally include active vMCP sessions and backend session ID mappings. - Add
ListActiveSessions()to the session manager and expose the data throughbuildStatusResponse. - Add unit tests for
ListActiveSessions()behavior and a regression test thatsessionsis omitted when V2 is disabled.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/server/status.go | Adds optional sessions block to /status when SessionManagementV2 is enabled. |
| pkg/vmcp/server/status_test.go | Updates status response test DTOs; adds tests for sessions omission and backend field redaction. |
| pkg/vmcp/server/sessionmanager/session_manager.go | Introduces SessionInfo and implements ListActiveSessions(). |
| pkg/vmcp/server/sessionmanager/list_sessions_test.go | Adds tests for listing behavior across empty, placeholder-only, and populated stores. |
| pkg/vmcp/server/session_manager_interface.go | Extends the SessionManager interface with ListActiveSessions(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4019 +/- ##
=======================================
Coverage 68.62% 68.63%
=======================================
Files 445 445
Lines 45374 45426 +52
=======================================
+ Hits 31140 31178 +38
- Misses 11827 11843 +16
+ Partials 2407 2405 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
size/M
github-actions
bot
added
size/L
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
Is this change really necessary? I get that it'd be nice to have visibility into active sessions, but exposing an endpoint seems heavy-handed. Alternative approaches:
- periodically log all the active sessions in memory
- log sessions as they are created and expired
What do you think?
yrobla
changed the title
github-actions
bot
added
size/S
Add session visibility to the /status endpoint for operational debugging and compliance auditing when sessionManagementV2 is enabled. Closes: #3876
github-actions
bot
added
size/S
ok this task came generated by claude, based on the rfc. A bit overkill, i agree. So i modified the issue to log content, as you were saying, and i modified the pr. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the /status endpoint session fields with structured logging: - CreateSession logs Info with session ID, backend count, and names - Terminate logs Info with session ID and backend count on expiry - StartPeriodicLogging starts a background goroutine that logs active session counts every minute via logActiveSessions() - Server.Start() wires up periodic logging when SessionManagementV2 is on Remove ListActiveSessions() from the SessionManager interface and drop the /status session fields (Sessions, SessionsStatus, BackendUsage). Closes: #3876 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> fix(vmcp): drop else after return in Terminate (revive lint) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
bot
added
size/S
|
@yrobla could you take a look at this draft PR and confirm if it’s still relevant? |
well, we agreed to implement it at some point, but it was not critical. If you don't mind, i prefer to keep them as draft, to resume the work when needed. |
Log session lifecycle events and periodic active session counts for operational debugging and compliance auditing when sessionManagementV2 is enabled.
Changes
Manager.CreateSession(): upgraded log toInfowith session ID, backend count, and backend namesManager.Terminate(): logInfowith session ID and backend count when a MultiSession expires; separate log for placeholder terminationManager.StartPeriodicLogging(ctx, interval): new method that starts a background goroutine logging active session counts every minuteManager.logActiveSessions(): internal helper that iterates sessions viaRange(), aggregates per-backend session counts, and logs a summary; logs a one-time warning when distributed storage makes enumeration unavailableServer.Start(): callsStartPeriodicLoggingwhenSessionManagementV2is enabledRemoved
/statusendpoint session fields (Sessions,SessionsStatus,BackendUsage) — session data is now surfaced through logs onlyListActiveSessions()fromSessionManagerinterfaceCloses: #3876